Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement full DkgSign over StackerDB #3911

Merged
merged 40 commits into from
Sep 18, 2023

Conversation

xoloki
Copy link
Collaborator

@xoloki xoloki commented Sep 13, 2023

Description

This PR builds on the test_stackerdb_dkg integration test, but now it uses libstackerdb and libsigner. It implements a full state-machine based stacks-signer, with all of the crypto and networking necessary to run a full DkgSign command.

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #3911 (99c1218) into feat/stacks-signer (3a998f6) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@                  Coverage Diff                   @@
##           feat/stacks-signer    #3911      +/-   ##
======================================================
- Coverage                0.16%    0.16%   -0.01%     
======================================================
  Files                     332      339       +7     
  Lines                  288692   290170    +1478     
======================================================
  Hits                      469      469              
- Misses                 288223   289701    +1478     
Files Changed Coverage Δ
libsigner/src/events.rs 0.00% <0.00%> (ø)
libsigner/src/http.rs 0.00% <0.00%> (ø)
libsigner/src/runloop.rs 0.00% <0.00%> (ø)
libsigner/src/session.rs 0.00% <0.00%> (ø)
libsigner/src/tests/mod.rs 0.00% <0.00%> (ø)
stacks-common/src/util/hash.rs 0.00% <0.00%> (ø)
stacks-signer/src/cli.rs 0.00% <0.00%> (ø)
stacks-signer/src/config.rs 0.00% <0.00%> (ø)
stacks-signer/src/crypto/frost.rs 0.00% <0.00%> (ø)
stacks-signer/src/crypto/mod.rs 0.00% <0.00%> (ø)
... and 8 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@xoloki xoloki self-assigned this Sep 13, 2023
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
libsigner/src/runloop.rs Outdated Show resolved Hide resolved
/// the 32 byte ECDSA private key used to sign blocks, chunks, and transactions
pub message_private_key: String,
/// The hex representation of the signer's Stacks private key
pub stacks_private_key: String,
Copy link
Member

@jcnelson jcnelson Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a hard time understanding the difference between message_private_key and stacks_private_key from just these comments. Can you help me understand why there need to be two private keys?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stacks_private_key controls money, message_private_key just lets you sign FROST messages. Users might have very different protection strategies for each.

One of my tasks for Nakomoto is to review how we store private keys, and offer various strategies for keeping them safe. Ideally we want to allow users to keep their stacks_private_keys in an HSM; all private keys should at least be trivially encrypted locally.

Having separate private keys from the start means we don't have to change this later.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can update the comment on the stacks_private_key as well to indicate what it is used for if that helps (communicating with the stacks node/stacker db contract)?

Copy link
Member

@jcnelson jcnelson Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the comment @jferrant!

@xoloki I agree in principle that the keys ought to be separate and have different storage strategies, but I'm curious why the signer needs to have a key that controls large amounts of STX in the first place? When the user stacks their STX in pox-4, they'd additionally set a dedicated signing key for the purposes of signing blocks, chunks, and transactions (i.e. the counterpart to message_private_key). The STX would presumably be managed by the user's wallet, not the signer. Additionally, the PoX rewards would be sent to a (separate) address of the user's choosing, which almost certainly would not be under the control of the signer. Please help me understand what I'm missing?

Copy link
Collaborator Author

@xoloki xoloki Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally possible that I'm making bad assumptions here. I though the way PoX worked was that users stacked significant (~100k) STX for each reward slot, and that the STX address associated with the stacking wallets would be the same STX address that Signers will use to talk to sbtc contracts etc.

If this is a bad assumption, and the STX address the Signer will use with stackerDB is not the address which controls the large STX stake, then yeah we can just use the STX private key for wsts message signing ECDSA too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The users don't stack to specific reward slots per se; they just stack a large quantity of STX, and depending on how much STX they stacked relative to everyone else, they are allotted zero or more reward slots by the system at the start of the next reward cycle.

That said, the key that holds the STX and performs the stacking operation would not need to be known to the signer. Instead, the act of stacking from this high-value key would assign a different block-signing key for the signer to use. Depending on what you think about this, it may even be desirable to have the user delegate two keys to the signer: one for signing Stacks blocks, and one for signing Bitcoin sBTC transactions. But in no case would the signer ever possess the key that controls the user's STX.

);
}
if self.ids_to_await.is_empty() {
// Calculate the aggregate signature
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to use a draining iterator for the following? This state won't be used again in this signing round, right? It would save you some clone()s and ensure this signature generation step only happens at most once.

}

/// Helper function to determine the slot ID for the provided stacker-db writer id and the message type
fn slot_id(id: u32, message: &MessageTypes) -> u32 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linking the comment I had on the other PR here for visibility: #3923 (comment)

@jcnelson
Copy link
Member

BTW CI is failing with: @xoloki

I get something similar when building on Alpine. It seems that CI needs to be updated to install libLLVM-16 development libraries and headers (in particular, the static libraries). It comes from p256k1.

@jcnelson
Copy link
Member

I only have three remaining concerns before approving:

  • Getting CI to pass (probably just need to install LLVM dev packages)
  • Trying to figure out why stacks_private_key is necessary
  • Trying to figure out if we need more slots in the StackerDB, or if we can get away with giving the signer only one slot (instead of N slots for each of the N signing states).

@xoloki
Copy link
Collaborator Author

xoloki commented Sep 15, 2023

  • Getting CI to pass (probably just need to install LLVM dev packages)

I'll fix this today.

  • Trying to figure out why stacks_private_key is necessary

See my comment above.

  • Trying to figure out if we need more slots in the StackerDB, or if we can get away with giving the signer only one slot (instead of N slots for each of the N signing states).

We don't need a set of slots per key_id/reward slot, we just need a set of slots per stacking entity/Signer. This will be more like 100, so with like 8 slots per Signer we only need like 800 slots total (currently we're using 16 slots per Signer but we don't need anywhere near that many). I think we'll be okay.

@jcnelson jcnelson self-requested a review September 15, 2023 18:26
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for your clarifications @xoloki! Once CI passes, feel free to merge 🙏

@xoloki
Copy link
Collaborator Author

xoloki commented Sep 15, 2023

Ugh, CI just keeps throwing more errors:

#11 598.3 Compiling stacks-node v0.1.0 (/build/testnet/stacks-node)
#11 653.7 LLVM ERROR: IO failure on output stream: No space left on device

@xoloki xoloki merged commit 5dee84f into feat/stacks-signer Sep 18, 2023
1 check passed
@xoloki xoloki deleted the chore/port-dkg-signer-first branch September 19, 2023 03:45
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants